Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: add argon2() and argon2Sync() methods #50353

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

ranisalt
Copy link

@ranisalt ranisalt commented Oct 24, 2023

Argon2 is a password-based key derivation function that is designed to expensive both computationally and memory-wise in order to make force attacks unrewarding.

OpenSSL added support for the Argon2 algorithm in the 3.2.0 (see openssl/openssl#12256). Add a js API modeled after crypto.scrypt() and crypto.scryptSync().

Related work:

Caveats:

  • OpenSSL v3.2.0 has not yet been released, so this is preliminary work based on the latest beta release (beta1 as of opening this pull request). To test this pull request, you must have OpenSSL 3.2 or later installed. Run ./configure with the following flags: --shared-openssl --shared-openssl-libpath /usr/lib/openssl-3.2/ --shared-openssl-includes /usr/include/openssl-3.2 (adjust accordingly)
  • It was not possible to enable passing the number of threads to the OpenSSL functions, Node.js hangs if you fire more than one async job at once. For now, it will run single threaded, which affects performance but results in the same hashes, so adding support later should be transparent to the user. solved

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 24, 2023
@khaosdoctor
Copy link
Member

cc @RafaelGSS @anonrig

@tniessen
Copy link
Member

I'm curious: now that EVP_KDF is available, should we design a generic KDF API instead of adding separate APIs for different KDFs? We've had unfortunate differences across such APIs in the past (see, for example, #39471). What are your thoughts @ranisalt @bnoordhuis @panva @jasnell?

@ranisalt
Copy link
Author

@tniessen how to handle the different parameters used by each KDF? At some point you will need separate functions to set the parameter list, but fetching the KDF and applying it can be merged.

Also, what's the status on having a standard "password hash" function (like PHP) that is transparent to the user? Strong defaults, no parameters besides password, and returning encoded string.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 24, 2023

But doesnt argon2 give different results based on parallelism? If so, doesnt this mean that if somebody uses argon2 for his product he is forced to not use parallelism. Later if it is decided they want to use parallelism, it is not backwards compatible?
Maybe should find a solution for parallelism to support this from start.

@tniessen
Copy link
Member

how to handle the different parameters used by each KDF? At some point you will need separate functions to set the parameter list

@ranisalt Various cryptographic libraries do this in more or less elegant ways. EVP_KDF is one such API. The entire SubtleCrypto interface of the Web Crypto API is also independent of the underlying algorithm. In JavaScript, we typically use objects with varying sets of properties to adapt to different algorithms. Another example is crypto.generateKeyPair(), which supports a variety of algorithms.

Also, what's the status on having a standard "password hash" function (like PHP) that is transparent to the user? Strong defaults, no parameters besides password, and returning encoded string.

I think that's a separate discussion. As long as Node.js provides the primitives (such as Argon2), I think this can be a userland library.

But doesnt argon2 give different results based on parallelism? If so, doesnt this mean that if somebody uses argon2 for his product he is forced to not use parallelism.

@Uzlopak Yes, the result may depend upon the parallelism parameter. No, that does not mean that users cannot use parallelism.

Users are responsible for choosing appropriate parallelism parameters, and parallelism can always be emulated through sequential operations. Please refer to scrypt(), which also has a parallelism parameter.

@ranisalt
Copy link
Author

But doesnt argon2 give different results based on parallelism? If so, doesnt this mean that if somebody uses argon2 for his product he is forced to not use parallelism. Later if it is decided they want to use parallelism, it is not backwards compatible?

Argon2 has two parallelism options, lanes and threads. Changing lanes will change the resulting hash, as it changes the underlying memory layout that is used. Changing threads, however, does not change the hash, it just allows operating in lanes (which are independent) in parallel. Usually, you want both to be the same, but it doesn't have to, as long as you have lanes >= threads. The Argon2 RFC itself doesn't even mention threads, only lanes, it's an implementation detail.

@tniessen thanks for the suggestions. I think that a generic KDF function would be excellent.

@panva
Copy link
Member

panva commented Oct 25, 2023

I'm curious: now that EVP_KDF is available, should we design a generic KDF API instead of adding separate APIs for different KDFs? We've had unfortunate differences across such APIs in the past (see, for example, #39471).

I'm sure supportive of such an API using EVP_KDF and crypto.kdf(identifier, { options }, callback) and crypto.kdfSync(identifier, { options }) would indeed be the signature I would expect.

Could even be, as we now have with crypto.sign, and crypto.verify, crypto.kdf(identifier, { options }, [callback]), just one method with the callback optional.

doc/api/crypto.md Outdated Show resolved Hide resolved
@ranisalt ranisalt marked this pull request as ready for review October 30, 2023 16:03
@ranisalt
Copy link
Author

ranisalt commented Nov 1, 2023

Added parallelism support by creating a new OpenSSL context for each hash. This is needed for the async mode, otherwise there is a race condition when one job finishes and resets the thread pool size to 0 while another job is still running.

@jasnell
Copy link
Member

jasnell commented Nov 11, 2023

I'm curious: now that EVP_KDF is available, should we design a generic KDF API instead of adding separate APIs for different KDFs?

Sorry I just noticed the mention on this. Yes, I'd be supportive of a generic API as opposed to protocol-specific APIs.

@ranisalt
Copy link
Author

Good news: OpenSSL 3.2 has finally been released

Bad news: there are incompatibilities with quictls' fork that won't be solved so soon.

@tniessen tniessen added the blocked PRs that are blocked by other issues or PRs. label Jan 3, 2024
@khaosdoctor
Copy link
Member

@nodejs/crypto does anyone know if this has moved forward in any way? As far as I've seen there is a PR that's being reviewed for a while, but I don't have any other details.

@targos
Copy link
Member

targos commented Jul 29, 2024

This is still blocked while we wait for an LTS release of OpenSSL that contains the new APIs.

@ranisalt
Copy link
Author

ranisalt commented Jul 29, 2024

OpenSSL has been launching LTS releases roughly every 3 years (2015, 2018, 2021) so it may be out soon, hopefully

I'm gonna rebase this PR soon

@richardlau
Copy link
Member

FWIW I've now added testing Node.js against OpenSSL 3.2 to our Jenkins CI: ubuntu2204_sharedlibs_openssl32_x64 (as part of node-test-commit-linux-containered).

For Node.js, the position hasn't changed -- we'll continue to bundle OpenSSL 3.0 as it is LTS and will review when OpenSSL announce what the next LTS version of OpenSSL will be. The new additional testing will at least allow early detection of potential issues for people building Node.js from source against external OpenSSL (e.g. downstream Linux distributions).

@TheOneTheOnlyJJ
Copy link

What's the status of this PR? It should be able to get merged now, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants